Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

• Affected method: CR1 cluster-robust sandwich vcov in Rust compute_robust_vcov_internal, reached by compute_robust_vcov and solve_ols(return_vcov=True).
• Methodology matches the registry/docstring contract: cluster meat remains sum_g score_g score_g' and CR1 adjustment remains (G/(G-1))*((N-1)/(N-k)).
• The change is a deterministic accumulation-order implementation detail, not a formula/default/assumption change.
• No NaN/Inf inference anti-patterns, control-group changes, parameter propagation issues, or security concerns found.
• One P3 test/documentation note: the new public-wrapper test factorizes “non-contiguous” IDs before Rust, so it does not directly test raw Rust non-contiguous IDs.

Methodology

No blocking findings.

Severity: N/A
Impact: The Rust code still computes the same CR1 cluster-robust sandwich specified in docs/methodology/REGISTRY.md:L126-L128 and diff_diff/linalg.py:L1693-L1696. The only changed behavior is deterministic cluster-row ordering before cluster_scores.t().dot(&cluster_scores) at rust/src/linalg.rs:L284-L318.
Concrete fix: None required.

Code Quality

No findings.

Severity: N/A
Impact: The new cluster_index map is only used for lookup, not iteration, so randomized HashMap ordering no longer affects vcov row order.
Concrete fix: None required.

Performance

No findings.

Severity: N/A
Impact: The change removes per-row to_owned() accumulation into map values and uses direct matrix row accumulation at rust/src/linalg.rs:L309-L314.
Concrete fix: None required.

Maintainability

No findings.

Severity: N/A
Impact: The Rust comment explains why deterministic ordering matters and preserves the CR1 formula context.
Concrete fix: None required.

Tech Debt

No findings.

Severity: N/A
Impact: No new deferred correctness limitation introduced.
Concrete fix: None required.

Security

No findings.

Severity: N/A
Impact: No secrets, unsafe code, or input-handling security concern introduced by the diff.
Concrete fix: None required.

Documentation/Tests

Severity: P3
Impact: The added test tests/test_rust_backend.py:L3535-L3545 imports diff_diff.linalg.compute_robust_vcov, whose wrapper factorizes cluster_ids before calling Rust at diff_diff/linalg.py:L1722-L1727. So the test covers public API non-contiguous IDs, but not raw _rust_backend.compute_robust_vcov receiving non-contiguous unsorted IDs directly.
Concrete fix: Optional: add a small raw Rust-backend regression importing diff_diff._rust_backend.compute_robust_vcov with the same [7, 3, 11, ...] cluster IDs and asserting repeated-call bit identity.

Tests not run in this read-only review sandbox.

🔁 AI review rerun (requested by @igerber)

Head SHA: 2f76a321bf0c0babfa73ae28e8c1839101f09645

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

• Affected method: Rust CR1 clustered sandwich vcov in compute_robust_vcov_internal, reached by _rust_backend.compute_robust_vcov and solve_ols(return_vcov=True).
• Methodology matches the registry/docstring contract: cluster meat remains Σ_g (X_g'u_g)(X_g'u_g)' with CR1 correction (G/(G-1))*((N-1)/(N-k)).
• The PR changes deterministic row aggregation order only; no estimator formula, default, identification assumption, inference field, or SE formula changes were found.
• The prior P3 test note remains: the public-wrapper “non-contiguous” test factorizes IDs before Rust, so it does not directly test raw Rust non-contiguous IDs.
• I could not run tests in this sandbox because numpy is unavailable.

Methodology

No blocking findings.

Severity: N/A
Impact: The changed Rust path still implements the CR1 contract documented in docs/methodology/REGISTRY.md:L126-L128 and diff_diff/linalg.py:L1694-L1696. The meat and adjustment at rust/src/linalg.rs:L385-L391 are unchanged in substance.
Concrete fix: None required.

Code Quality

No findings.

Severity: N/A
Impact: cluster_index is used only for deterministic lookup, not iteration, so HashMap randomized iteration order no longer affects the cluster-score row order at rust/src/linalg.rs:L361-L383.
Concrete fix: None required.

Performance

No findings.

Severity: N/A
Impact: The change removes per-observation to_owned() row allocation and direct map-value array replacement, replacing it with direct accumulation into cluster_scores.
Concrete fix: None required.

Maintainability

No findings.

Severity: N/A
Impact: The Rust comment explains why deterministic aggregation is needed and keeps the CR1 formula nearby.
Concrete fix: None required.

Tech Debt

No findings.

Severity: N/A
Impact: No new deferred correctness limitation or untracked silent behavior change found.
Concrete fix: None required.

Security

No findings.

Severity: N/A
Impact: No secrets, unsafe code, or security-sensitive input handling changes found.
Concrete fix: None required.

Documentation/Tests

Severity: P3
Impact: The prior informational test gap remains. tests/test_rust_backend.py:L3647-L3657 imports diff_diff.linalg.compute_robust_vcov; that wrapper factorizes cluster IDs before Rust at diff_diff/linalg.py:L1785-L1790. So the test covers public API determinism for non-contiguous original labels, but not raw _rust_backend.compute_robust_vcov receiving non-contiguous unsorted IDs directly.
Concrete fix: Optional: add a small direct _rust_backend.compute_robust_vcov regression with raw IDs like [7, 3, 11, ...] and assert repeated-call bit identity.

🔁 AI review rerun (requested by @igerber)

Head SHA: 5aa51c71d909ea222f877fe49f8631c39e7f81de

Overall Assessment

✅ Looks good — no unmitigated P0/P1 findings.

Executive Summary

• Affected method: Rust CR1 clustered sandwich vcov in rust/src/linalg.rs:342-L397, reached through compute_robust_vcov and solve_ols(return_vcov=True).
• Methodology matches the registry/docstring contract: CR1 meat remains Σ_g (X_g'u_g)(X_g'u_g)' with (G/(G-1))*((N-1)/(N-k)).
• The change removes HashMap iteration from the cluster-score row order; the map is now used only for deterministic cluster-index lookup.
• The prior P3 raw-kernel test gap is addressed by tests/test_rust_backend.py:L3661-L3676.
• I could not run tests locally: pytest/numpy are unavailable, and cargo check was blocked by restricted network access to crates.io.

Methodology

Finding: None.
Severity: N/A
Impact: The changed Rust path still implements the CR1 formula documented in docs/methodology/REGISTRY.md:L126-L128 and diff_diff/linalg.py:L1694-L1696. The meat and finite-sample adjustment remain unchanged at rust/src/linalg.rs:L385-L391; only row accumulation order changed.
Concrete fix: None required.

Code Quality

Finding: None.
Severity: N/A
Impact: HashMap randomized iteration no longer controls the (G, k) cluster-score matrix order. cluster_index is populated deterministically by first appearance and then used for direct row accumulation at rust/src/linalg.rs:L361-L383.
Concrete fix: None required.

Performance

Finding: None.
Severity: N/A
Impact: The implementation removes per-row to_owned() cluster sum allocation/replacement and accumulates directly into cluster_scores.
Concrete fix: None required.

Maintainability

Finding: None.
Severity: N/A
Impact: The Rust comment explains why deterministic aggregation matters and keeps the CR1 formula adjacent to the computation.
Concrete fix: None required.

Tech Debt

Finding: None.
Severity: N/A
Impact: No new deferred correctness limitation or silent behavioral change found.
Concrete fix: None required.

Security

Finding: None.
Severity: N/A
Impact: No secrets, unsafe code, or security-sensitive input handling changes found in the diff.
Concrete fix: None required.

Documentation/Tests

Finding: None.
Severity: N/A
Impact: The changelog documents the deterministic clustered-vcov fix at CHANGELOG.md:L38-L47. The prior informational gap is resolved: tests/test_rust_backend.py:L3661-L3676 directly exercises _rust_backend.compute_robust_vcov with unsorted, non-contiguous raw int64 cluster IDs.
Concrete fix: None required.